-
Notifications
You must be signed in to change notification settings - Fork 823
Add active series limit for nativeHistogram samples #6796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add active series limit for nativeHistogram samples #6796
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall changes looks good. Please add tests.
@@ -1,6 +1,7 @@ | |||
# Changelog | |||
|
|||
## master / unreleased | |||
* [ENHANCEMENT] Ingester: Add activeSeries limit specifically for NativeHistograms. #6796 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changelog item needs to be grouped with other enhancement items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Updated.
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
024c1e9
to
ab7970d
Compare
@@ -3606,6 +3606,11 @@ The `limits_config` configures default and per-tenant limits imposed by Cortex s | |||
# CLI flag: -ingester.max-series-per-metric | |||
[max_series_per_metric: <int> | default = 50000] | |||
|
|||
# The maximum number of active native histogram series per user, per ingester. 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to the description that this limit needs active series tracker to be enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks updated.
Added check of activeSeriesMetricsEnabled to the earlier MaxGlobalNativeHistogramSeriesPerUser validation of shardByAllLabels. and to the MaxLocalNativeHistogramSeriesPerUser
pkg/ingester/ingester.go
Outdated
@@ -448,6 +448,11 @@ func (u *userTSDB) PreCreation(metric labels.Labels) error { | |||
} | |||
} | |||
|
|||
// Total nativeHistograms series limit. | |||
if err := u.limiter.AssertMaxNativeHistogramSeriesPerUser(u.userID, u.activeSeries.ActiveNativeHistogram()); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can activeSeries be nil? Should we handle for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think null check should not be required as preallocation is done while initialization: https://github.com/cortexproject/cortex/blob/master/pkg/ingester/active_series.go#L44
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
Signed-off-by: Paurush Garg <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you.
What this PR does:
In addition to the current activeSeries limit on float+histogram samples, this PR adds an active series limit for specifically for nativeHistogram
samples.
This is done, b/c the ingestion of native histogram samples is much more CPU intensive than that of float samples - adding nativeHistogram samples specific ingestions limit to protect the service and to allow clients to adjust the NH series ingestion.
Currently, Prometheus doesn't provide a way to count the specific active nativeHistogram series in the head and hence this PR uses the activeNativeHistograms series count from Cortex - which differs in the way activeSeries is counted.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]